📸 Snapshot Test

Error generating snapshot comparison

🛸 Powered by Emerge Tools

@RCGitBot please test

@claude please review

Claude finished @fire-at-will's task in 2m 8s —— View job

Code Review

• Read repository guidelines (CLAUDE.md)
• Review core implementation (SandboxEnvironmentDetector.swift)
• Review Purchases.swift integration
• Review DangerousSettings changes
• Review test coverage and implementation
• Provide comprehensive feedback

Summary

This PR implements a significant improvement to sandbox environment detection by fetching the environment from the StoreKit 1 receipt instead of relying solely on receipt URL path detection. The implementation includes proper prefetching, fallback mechanisms, and comprehensive test coverage.

Positive Aspects

1. Well-designed fallback chain: The priority order (simulator → prefetched receipt → legacy path) is logical and well-tested
2. Async prefetching: Good use of background threads to avoid blocking the main thread
3. Comprehensive test coverage: Excellent test coverage with 491 new test lines covering both iOS and macOS scenarios
4. Thread safety: Good use of Atomic types for thread-safe access to cached values
5. Integration testing: Proper use of testSandboxEnvironmentDetector in dangerous settings for integration tests

Issues & Concerns

1. Race Condition in cacheIsSandboxFromLocalReceiptEnvironment() (Critical)

Location: Sources/Misc/SandboxEnvironmentDetector.swift:143-158

There's a potential race condition when switching from worker thread to main actor:

func cacheIsSandboxFromLocalReceiptEnvironment() {
    operationDispatcher.dispatchOnWorkerThread {
        // Parsing happens on worker thread
        guard let environment = try? self.receiptFetcher.fetchAndParseLocalReceipt().environment else {
            return
        }

        guard environment != .unknown else {
            return
        }

        // Switch to main actor to update cache
        self.operationDispatcher.dispatchOnMainActor {
            self.cachedIsSandboxFromPrefetchedReceipt.value = environment != .production
        }
    }
}

Problem: While Atomic types provide thread safety for individual reads/writes, multiple callers could read isSandbox while this value is being computed. Since Atomic<Bool?> starts as nil, early callers might fall through to legacy detection even after prefetching starts, leading to inconsistent results during the race window.

Recommendation: This is likely acceptable given that:

• Early callers will still get a correct (though potentially less accurate) result via fallback
• Once the cache is populated, all subsequent calls use the cached value
• The race window is small (only during initial app launch)

However, consider adding a comment explaining this behavior is intentional.

2. Simulator Check Precedence May Mask Issues

Location: Sources/Misc/SandboxEnvironmentDetector.swift:81-84

var isSandbox: Bool {
    guard !self.isRunningInSimulator else {
        return true  // Always returns true for simulator
    }
    // ...
}

Concern: Always returning true for simulator means that if there are bugs in the prefetching logic, they won't be caught in simulator testing. This is probably the correct behavior for production, but consider whether integration tests should have a way to bypass this check to test the actual prefetching logic in simulators.

Current State: Tests do handle this correctly by passing inSimulator: false to test the prefetching logic.

3. Missing Documentation on shouldPrefetchReceiptEnvironment Behavior

Location: Sources/Misc/SandboxEnvironmentDetector.swift:59

The parameter shouldPrefetchReceiptEnvironment is set to false when apiKeyValidationResult == .simulatedStore (in Purchases.swift:372), but there's no documentation explaining why simulated stores should skip prefetching.

Recommendation: Add a comment explaining that simulated stores don't have real receipts, so prefetching would fail or be unnecessary.

4. Silent Failure in Receipt Parsing

Location: Sources/Misc/SandboxEnvironmentDetector.swift:146-148

guard let environment = try? self.receiptFetcher.fetchAndParseLocalReceipt().environment else {
    return  // Silently fails
}

Concern: Receipt parsing failures are silently ignored. While this is likely intentional (to gracefully fall back to legacy detection), consider whether at least a debug log would be helpful for troubleshooting.

Note: The macOS-specific code path in isProductionReceipt (line 196) does log errors, but the prefetch path doesn't.

5. SystemInfo._isSandbox Lazy Variable Removed

Location: Sources/Misc/SystemInfo.swift:84-86

The lazy variable was removed in favor of direct access to sandboxEnvironmentDetector.isSandbox. This is good because:

• The old lazy variable would only compute once, missing any updates from prefetching
• Direct access ensures dynamic behavior

However, this means isSandbox is now computed on every access. Since SandboxEnvironmentDetector caches results internally, this should be fine, but it changes the performance characteristics slightly.

Recommendation: Consider adding a comment explaining why the lazy variable was removed.

6. Default Instance Pattern is Unusual

Location: Sources/Misc/SandboxEnvironmentDetector.swift:110-115

private static let _default: Atomic<SandboxEnvironmentDetectorType> = .init(SandboxEnvironmentDetector())

static var `default`: SandboxEnvironmentDetectorType {
    get { _default.value }
    set { _default.value = newValue }  // Mutable static singleton
}

Concern: A mutable static singleton is unusual and could lead to unexpected behavior if multiple parts of the code try to set different detectors. However, looking at the usage, this appears to be set exactly once during Purchases.configure() and then used everywhere, so it should be safe.

Current State: The pattern is documented as being for testing purposes (DangerousSettings.testSandboxEnvironmentDetector), which is reasonable.

7. StoreKit2TransactionFetcher Integration Not Used

Location: Sources/Purchasing/StoreKit2/StoreKit2TransactionFetcher.swift:182-186

The PR adds appTransactionEnvironment to StoreKit2TransactionFetcher, but I don't see it being used in the actual sandbox detection logic. This suggests it might be for future use or is part of the foundation from PR #6154.

Question: Is this intentional for future use, or should it be integrated into the current detection logic?

Minor Issues

1. Test Helper Complexity: The test helpers in SandboxEnvironmentDetectorTests.swift are quite complex with multiple mock objects and stalled states. While comprehensive, they may be hard to maintain. Consider adding documentation comments to the helper functions.

2. Integration Test Pattern Change: The change from static isSandbox to instance variable in BaseBackendIntegrationTests is good, but the didSet observer was removed. Ensure this doesn't break any test setup that depended on that side effect.

Recommendations Summary

1. Add a comment in cacheIsSandboxFromLocalReceiptEnvironment() explaining the race condition behavior is acceptable
2. Add documentation explaining why simulated stores skip prefetching
3. Consider adding debug logging for receipt parsing failures in the prefetch path
4. Add a comment explaining why the lazy _isSandbox variable was removed from SystemInfo
5. Clarify whether appTransactionEnvironment is for future use or should be integrated now

Verdict

This is a solid PR with good architecture and excellent test coverage. The issues identified are mostly minor concerns about documentation and edge cases. The race condition in point #1 is worth considering, but appears to be acceptable given the fallback mechanism. The implementation properly handles the complexity of async prefetching with appropriate fallbacks.

Approval Status: The code is well-implemented and ready to merge with minor documentation improvements.

|

Note: I'll remove the AppTransaction additions to the StoreKit2TransactionFetcher tomorrow since they aren't used here 👍